Skip to content

refactor: replace 3rd-party xxhash impl with in-house reimpl#2310

Merged
0xFA11 merged 2 commits intodevelopfrom
refactor/xxhash
Nov 15, 2022
Merged

refactor: replace 3rd-party xxhash impl with in-house reimpl#2310
0xFA11 merged 2 commits intodevelopfrom
refactor/xxhash

Conversation

@0xFA11
Copy link
Copy Markdown
Contributor

@0xFA11 0xFA11 commented Nov 15, 2022

This PR replaces 3rd-party xxHash32 & xxHash64 implementations with in-house full reimplementation.

Changelog

  • Changed 3rd-party XXHash (32 & 64) implementation with an in-house reimplementation

Testing and Documentation

  • Added XXHashTests unit tests

@0xFA11 0xFA11 requested a review from a team as a code owner November 15, 2022 02:25

namespace Unity.Netcode.EditorTests
{
public class XXHashTests
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never had unit tests to verify hash results, I implemented them with some hardcoded strings and expected hardcoded results.

Having said that, even though it's a full reimplementation of the 3rd-party one, they both give us the exact same results as expected as they are both completely valid and accurate implementations — we're just getting rid of our external dependency and also making them just slightly more performant/efficient.

@0xFA11 0xFA11 enabled auto-merge (squash) November 15, 2022 02:30
Copy link
Copy Markdown
Member

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
👍

@0xFA11 0xFA11 merged commit f1870cd into develop Nov 15, 2022
@0xFA11 0xFA11 deleted the refactor/xxhash branch November 15, 2022 19:22
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants